Migrate kubectl_apply_manifest module to helm#5282
Migrate kubectl_apply_manifest module to helm#5282agrawalkhushi18 wants to merge 15 commits intoGoogleCloudPlatform:developfrom
Conversation
Summary of ChangesHello @agrawalkhushi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Kubernetes manifest application process by migrating from a custom kubectl provider to the official hashicorp/helm provider. This change enhances the reliability and stability of manifest deployments, simplifies the handling of various manifest sources, and introduces robust atomic operations with automatic rollbacks, leading to a more predictable and resilient cluster management experience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the kubectl-apply module to use the hashicorp/helm provider instead of the gavinbunney/kubectl provider for applying manifests. This is a significant improvement that addresses limitations like the annotation size for large CRDs by leveraging Helm's release state storage. The introduction of unique release names using random_id and enabling atomic operations by default also enhances the robustness and reliability of applying manifests.
The review has identified a couple of areas for improvement. The logic for scanning directories of manifests is a bit too broad and could lead to errors if non-manifest files are present. Additionally, the namespace for Helm releases is hardcoded, which reduces the flexibility of the module compared to the previous implementation.
It's also worth noting that while the PR description states the gavinbunney/kubectl provider is removed entirely, it appears to still be in use by the install_asapd_lite part of this module. To fully complete the migration, this could also be updated to use the new Helm-based approach in a future change.
5ae1f78 to
ba70406
Compare
tools/cloud-build/daily-tests/ansible_playbooks/test-validation/test-gke-a3-high.yml
Outdated
Show resolved
Hide resolved
tools/cloud-build/daily-tests/ansible_playbooks/test-validation/test-gke-a3-high.yml
Outdated
Show resolved
Hide resolved
shubpal07
left a comment
There was a problem hiding this comment.
Thanks for putting this together! Migrating from raw kubectl apply to Helm is a great architectural move that will really improve state management and reliability for these manifests. Left a few inline comments.
SwarnaBharathiMantena
left a comment
There was a problem hiding this comment.
Is the kubectl sub-folder going to be cleaned out or is there some other existing dependency on it?
It can be cleaned out once the migration proves to be robust enough after all the kubectl_apply modules have been transitioned to use helm provider. We can monitor the tests for 1 month and then deprecate it. |
…tions in slurm-gke.yaml
77ecd95 to
7a6e360
Compare
This PR migrates the
kubectl_apply_manifestmodule from thegavinbunney/kubectlprovider to the officialhashicorp/helmprovider.Key Changes
1. Re-implementation of
kubectl_apply_manifestshashicorp/helmvia a localraw-config-chart.2. Enhanced Manifest Processing in
localslocalsblock..tftpltemplates, raw files, and directories) into a single rendered string before passing it to Helm.3. Server-Side Apply (SSA) Deprecation
server_side_applysupport as it is no longer necessary. Few blueprints have been modified accordingly.kubectl.kubernetes.io/last-applied-configurationannotation that necessitated SSA for large CRDs.4. Release Naming Stability
random_idresource to generate a unique 4-byte suffix for each Helm release (e.g.,manifest-apply-ceab0dfc-0).hashicorp/randomtoversions.tfto support conflict-free release naming.gke-cluster,gke-node-pool, and user workloads) instantiatekubectl-applysimultaneously within the same blueprint. The 4-byte length was chosen to guarantee uniqueness while staying comfortably within Kubernetes' 53-character name limit.5. Atomic Operations
atomic = trueby default on the Helm release.NOTE:
gke-nodesetCustom Resources were being applied before the required slinky Helm CRDs finished installing. By anchoring explicitdepends_onconstraints to the slinky module outputs, we force the Terraform DAG to correctly serialize both the creation and destruction phases of the deployment.Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.